-
Notifications
You must be signed in to change notification settings - Fork 730
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: separate podlabels in controller-manager and audit deployment #3378
base: master
Are you sure you want to change the base?
feat: separate podlabels in controller-manager and audit deployment #3378
Conversation
0b23d4f
to
23d1737
Compare
23d1737
to
3b6e6b1
Compare
obj = strings.Replace(obj, " priorityClassName: system-cluster-critical", " {{- if .Values.audit.priorityClassName }}\n priorityClassName: {{ .Values.audit.priorityClassName }}\n {{- end }}", 1) | ||
obj = strings.Replace(obj, " - emptyDir: {}", " {{- if .Values.audit.writeToRAMDisk }}\n - emptyDir:\n medium: Memory\n {{ else }}\n - emptyDir: {}\n {{- end }}", 1) | ||
} | ||
|
||
if kind == DeploymentKind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @bobertrublik! We need to ensure backward compatibilities as to not break existing things users might be using and reduce developer toil. can you pls bring back all the existing podLabels and add support for new audit and controller manager podLabels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, I'm just a little unsure how to approach this. Are you saying that I should readd the .Values.podLabels
field to ensure backward compatibility? Are the labels set there added to all pods and we can additionally set the .Values.controllerManager.podLabels
and .Values.audit.podLabels
to add specific ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, Pod can have many labels. Please restore the existing values.podLabels field to ensure backward compatibility. Then add the new labels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restored .values.podLabels
and kept .Values.controllerManager.podLabels
and .Values.audit.podLabels
. 🙂
dacf5ee
to
5be0b6a
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3378 +/- ##
==========================================
- Coverage 54.49% 46.76% -7.74%
==========================================
Files 134 218 +84
Lines 12329 14784 +2455
==========================================
+ Hits 6719 6913 +194
- Misses 5116 7070 +1954
- Partials 494 801 +307
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Robert Bublik <mail@robertbublik.de>
5be0b6a
to
f6b92d8
Compare
Hello @ritazh, I rebased the branch onto main and wanted to ask you for a short review and approval. 🙂 |
What this PR does / why we need it:
We want to introduce new labels across our infrastructure and validate them using the
K8sRequiredLabels
policy. Depending on its workload each pod will receive a differentworkload
label which is then used to enrich metrics collected from it. The Gatekeeper Helm chart doesn't allow setting separate labels for the controller manager pod and audit pod which will break our idea.Special notes for your reviewer:
I was following the PR below to apply the changes here.
I had to remove
{{- include "gatekeeper.podLabels" . | nindent 8 }}
across all jobs since it's removed from the helpers.tpl file.